Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add pallet connectors gateway #1376

Closed
wants to merge 35 commits into from
Closed

Conversation

cdamian
Copy link
Contributor

@cdamian cdamian commented May 30, 2023

Description

This PR adds the Connectors Gateway pallet which will be used to process inbound and outbound Connectors messages.

Changes and Descriptions

  • Connectors Gateway pallet - entrypoint for Connectors messages that's using the InboundQueue for processing incoming messages in the case of successful validation, and that implements the OutboundQueue for processing outgoing messages that are handled by the according XCM or EVM router.
  • Connectors Gateway routers - separate crate that's part of the Connectors Gateway pallet which holds the routers that are used in the gateway.

Checklist:

  • I have added Rust doc comments to structs, enums, traits and functions
  • I have made corresponding changes to the documentation
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or that my feature works

Copy link
Contributor

@lemunozm lemunozm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of comments about mocking, but looks quite good!

libs/mocks/src/connectors.rs Outdated Show resolved Hide resolved
Comment on lines +4 to +33
#[derive(Debug, Eq, PartialEq)]
pub enum MessageMock {
First,
Second,
}

impl MessageMock {
fn call_type(&self) -> u8 {
match self {
MessageMock::First => 0,
MessageMock::Second => 1,
}
}
}

impl Codec for MessageMock {
fn serialize(&self) -> Vec<u8> {
vec![self.call_type()]
}

fn deserialize<I: Input>(input: &mut I) -> Result<Self, Error> {
let call_type = input.read_byte()?;

match call_type {
0 => Ok(MessageMock::First),
1 => Ok(MessageMock::Second),
_ => Err("unsupported message".into()),
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm going too far, but maybe a simple number can be used as a message to avoid this type?

An later use something like:

domain_router.send(sender, 42).unwrap()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That simple type wouldn't implement the Codec trait, some boilerplate might still be needed regardless.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmm... that's weird 🤔 . I'm pretty sure Codec and Serialize is implemented for basic types. Could it be because the compiler doesn't know exactly what that 42 is? Sometimes in these cases, you need to help the compiler with something like 42u32.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are probably referring to a different Codec trait, I'm referring to the one that we have in connectors - https://github.com/centrifuge/centrifuge-chain/blob/add-pallet-connectors-gateway/libs/traits/src/connectors.rs#L19

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch, you're right! Never mind then

Copy link
Contributor

@lemunozm lemunozm Jun 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although... would it make sense to add the basic types to the Codec trait along with the implementation (as it happens with the substrate Codec and Serialize traits)? @NunoAlexandre @wischli

@mustermeiszer
Copy link
Collaborator

Coming back to this: #1403 (comment)

I think we will just benchmark the call as we usually do. IIUC this will result in the caller having to pay fees twice. For now I think it is fine, we must just create a todo to fix this!

The only thing we must ensure is that these lines

// Use the same conversion as the one used in `EnsureAddressTruncated`.
let sender_evm_address = H160::from_slice(&sender.as_ref()[0..20]);

create the backwards conversion to the mapping that type AddressMapping: AddressMapping<Self::AccountId>; is doing. So we need roundtripping for that.

cc @branan for consolidation.

Copy link
Contributor

@NunoAlexandre NunoAlexandre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One suggestion 👀

pallets/connectors-gateway/src/lib.rs Outdated Show resolved Hide resolved
pallets/connectors-gateway/src/lib.rs Outdated Show resolved Hide resolved
@cdamian cdamian force-pushed the add-pallet-connectors-gateway branch from 2452e2f to 241d0e2 Compare July 11, 2023 13:38
mustermeiszer
mustermeiszer previously approved these changes Jul 13, 2023
Copy link
Collaborator

@mustermeiszer mustermeiszer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@mustermeiszer
Copy link
Collaborator

Failing tests can be fixed by applying the latest changes of the precompile branch here. I just do not wnat the node changes in this PR. THe latest precompile stuff can already be included.

cdamian added 14 commits July 13, 2023 20:56
* gateway: Move domain routers to runtime

* gateway-routers: Move to separate crate

* runtime: Remove unused deps

* traits: Add more comments to Router trait, rename forward to send

* connectors-gateway: Rename routers crate, use ensure! for validation

* connectors: Rename submit to handle

* connectors-gateway: Drop max message const, use Vec instead

* connectors-gateway: Rename router and queues methods

* connectors-gateway: Update edition to 2021

* gateway-routers: Drop wildcard import

* connectors-gateway: Rename DomainSubmitters to ConnectorsAllowlist and associated types

* connectors-gateway: Add missing features

* connectors-gateway: Update deps and runtime config
* pallets: Add Ethereum Transaction pallet

* ethereum-transaction: Drop origin

* ethereum-transaction: Drop weights and origin type

* ethereum-transaction: Handle call info exit reasons
@cdamian cdamian force-pushed the add-pallet-connectors-gateway branch from 40434f5 to ac8cb63 Compare July 13, 2023 17:56
@cdamian cdamian marked this pull request as ready for review July 13, 2023 18:03
@mustermeiszer mustermeiszer added the D0-ready Pull request can be merged without special precaution and notification. label Jul 13, 2023
Copy link
Collaborator

@mustermeiszer mustermeiszer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not an perfect in debth review, but I have seen this PR before and it still looks good. Great work again @cdamian!

But would be good to get another pair of eyes on this at least! ^^

@cdamian
Copy link
Contributor Author

cdamian commented Jul 14, 2023

This PR will be split into multiple smaller PRs that focus on one part.

@cdamian
Copy link
Contributor Author

cdamian commented Jul 14, 2023

Part 1 - adding Connectors Gateway pallet - #1456
Part 2 - adding Ethereum Transaction pallet - #1460
Part 3 - adding Connectors Gateway routers - #1475
Part 4 - integration tests for all of the above - #1478

@cdamian cdamian marked this pull request as draft July 14, 2023 11:47
@NunoAlexandre
Copy link
Contributor

@cdamian do we want to close this?

@cdamian
Copy link
Contributor Author

cdamian commented Jul 31, 2023

@cdamian do we want to close this?

@NunoAlexandre Let's keep it open until we merge all remaining parts, it's also helpful for me to keep track of things ^^

@NunoAlexandre
Copy link
Contributor

@cdamian do we want to close this?

@NunoAlexandre Let's keep it open until we merge all remaining parts, it's also helpful for me to keep track of things ^^

Sure, I was just trying to follow these gateway-related PRs an given your comments [https://github.com//pull/1376#issuecomment-1635746946] above I thought this PR was now obsolete given those two Parts have been merged already 👍

@cdamian
Copy link
Contributor Author

cdamian commented Jul 31, 2023

@cdamian do we want to close this?

@NunoAlexandre Let's keep it open until we merge all remaining parts, it's also helpful for me to keep track of things ^^

Sure, I was just trying to follow these gateway-related PRs an given your comments [https://github.com//pull/1376#issuecomment-1635746946] above I thought this PR was now obsolete given those two Parts have been merged already +1

I will document all following PRs here then close it when everything is done.

@NunoAlexandre
Copy link
Contributor

@cdamian do we want to close this?

@NunoAlexandre Let's keep it open until we merge all remaining parts, it's also helpful for me to keep track of things ^^

Sure, I was just trying to follow these gateway-related PRs an given your comments [https://github.com//pull/1376#issuecomment-1635746946] above I thought this PR was now obsolete given those two Parts have been merged already +1

I will document all following PRs here then close it when everything is done.

Sweet, thanks and sorry for the unintentional noise :)

@cdamian
Copy link
Contributor Author

cdamian commented Aug 10, 2023

Closing this since the last related PR was just merged. Other work related to the topic, such as the precompiles will be done separately and documented accordingly.

@cdamian cdamian closed this Aug 10, 2023
@cdamian cdamian deleted the add-pallet-connectors-gateway branch July 30, 2024 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D0-ready Pull request can be merged without special precaution and notification.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants